-
Notifications
You must be signed in to change notification settings - Fork 34
Stop looking for primaries for simple radiative photon-like geometries #2178
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
wdconinc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks sensible.
ruse-traveler
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch!
mhkim-anl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks sensible to me as well
c1133e1
…like radiative photons from a primary electrons
7205da0 to
5f19d59
Compare
ruse-traveler
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still good!
|
The test https://github.com/eic/EICrecon/blob/main/src/tests/algorithms_test/calorimetry_CalorimeterClusterRecoCoG.cc was written exactly to demonstrate the behaviour that was coded by Derek in the original version. I think we can fix the test in a straightforward way. diff --git a/src/tests/algorithms_test/calorimetry_CalorimeterClusterRecoCoG.cc b/src/tests/algorithms_test/calorimetry_CalorimeterClusterRecoCoG.cc
index 62b22de08..e266b54c6 100644
--- a/src/tests/algorithms_test/calorimetry_CalorimeterClusterRecoCoG.cc
+++ b/src/tests/algorithms_test/calorimetry_CalorimeterClusterRecoCoG.cc
@@ -230,15 +230,18 @@ TEST_CASE("the calorimeter CoG algorithm runs", "[CalorimeterClusterRecoCoG]") {
REQUIRE(clust_coll->size() == 1);
auto clust = (*clust_coll)[0];
- REQUIRE(assoc_coll->size() == 2);
+ REQUIRE(assoc_coll->size() == 3);
// Half of the energy comes from mcpart11 and its daughter mcpart12
- REQUIRE_THAT((*assoc_coll)[0].getWeight(), Catch::Matchers::WithinAbs(0.5, EPSILON));
+ REQUIRE_THAT((*assoc_coll)[0].getWeight(), Catch::Matchers::WithinAbs(0.25, EPSILON));
REQUIRE((*assoc_coll)[0].getRec() == clust);
REQUIRE((*assoc_coll)[0].getSim() == mcpart11);
+ REQUIRE_THAT((*assoc_coll)[1].getWeight(), Catch::Matchers::WithinAbs(0.25, EPSILON));
+ REQUIRE((*assoc_coll)[1].getRec() == clust);
+ REQUIRE((*assoc_coll)[1].getSim() == mcpart12);
// Half of the energy comes from mcpart2
- REQUIRE_THAT((*assoc_coll)[1].getWeight(), Catch::Matchers::WithinAbs(0.5, EPSILON));
- REQUIRE((*assoc_coll)[1].getRec() == clust);
- REQUIRE((*assoc_coll)[1].getSim() == mcpart2);
+ REQUIRE_THAT((*assoc_coll)[2].getWeight(), Catch::Matchers::WithinAbs(0.5, EPSILON));
+ REQUIRE((*assoc_coll)[2].getRec() == clust);
+ REQUIRE((*assoc_coll)[2].getSim() == mcpart2);
} |
|
Regarding the failing test, I'm not so sure anymore what behavior we want here as it depends. I can make the lookup code configurable in maximum number of steps below the "root" particle it goes, then the setup in CoG doesn't need to match the one in the SimCalorimeterHitProcessor anymore. I'll wait for @ruse-traveler to weigh in on the desired behavior. We can quite trivially make it configurable if need be. |
1a0ee98
In the test we have a situation where an electron emits a roughly collinear photon during the simulation stage and both contribute to a single hit. Then a pi0 contributes to a separate hit, and the 2 hits are grouped into a cluster. In this case, my thinking is that we would want associations back to only the electron & pi0 since the photon here is a secondary (presumably generated by an interaction with the trackers or services). But if the photon was radiated during the generation stage, this would presumably happen before we had any chance to measure the electron before the radiation. In this case, I would think we could consider both as proper "primaries" and we would want associations back to all 3... All that being said, I do think making it configurable would be really useful! Our definition of primary and gen-reco mappings are still very much WIPs after all. |
|
To be honest, I'm surprised that the change picks up all 3 since the photon has a generator status of 0🤨 |
|
Today in the BIC simulation meeting I was wondering about two questions:
The proposal is that the proposed implementation could be used to do studies using private simulations, then you can confirm for yourself if the algorithm is viable. In addition, it would help to understand this by having a unit test available with demonstration of some cases that were intended. |
Ah! Apologies! I was being a dummy: this is the intended behavior, since the goal is to associate to the electron and photon after the radiation. |
|
Hi Folks - see below:
That is not what we try to avoid - ambiguity within a single hit likely should be tracked to the parent. However, there are 2 places where this is relevant:
Not sure I understand this question.
I wonder if that really happens, as in general showers shouldn't happen outside the tracking volume. Of course, sometimes they do (e.g. in the support cone). Is your worry that some showers are associated with the primary particle, but a very small fraction where the first 2 particles happen to be tracked it would be associated to those?
I think there's a way to serve all cases, I need to think a bit about it as it's not immediately obvious what the best way out is (without making everything more complicated that it has to be). |
I was also being foolish here: the intent of the change, of course, is that we're no longer associating to just primaries but also to relevant secondaries. With this change, we should associate to each. But following up on this and the BIC simulation meeting today, let me elaborate a little on what the intended behavior was with the current association scheme. When we switched to it, the primary goal was to reflect the fact that multiple particles can contribute to a single cluster. The selection of only primaries (i.e.
But this clearly leaves a lot to be desired (e.g. splash from the beampipe in the LFHCAL is completely absent in this scheme): it's very coarse-grained. I think the change here moves in the right direction by expanding associations to include (relevant) secondaries. |
These are very good reasons, we want to enable all these studies. There might be caveats that I discuss below.
Above you were suggesting that CoG (Clustering) configuration nee not match the configuration in SimCalorimeterHitProcessor (Hits), and I see there is either a commonality or a difference that we'd want to explain here.
My impression was that when we developed the original code people were reporting seeing interaction vertices outside of the tracking region. I think, and @ruse-traveler can correct me on that, no one at the time understood what exactly was the logic DD4hep was following, and that was the reason why the final algorithm ended up being so simple. There is also a valid reason why we might want to extend the tracking region to the faces of the hadronic calorimeters for some types of analysis. Maybe all of this can be addressed by an algorithm that is aware of not only the topology of the event record, but also the vertex positions themselves (and/or particle types).
Okay, maybe now I get that what you are doing serves all cases, we just need to better investigate how to actually do what you want. |
|
Forgot to add to point 1. I guess, you are not strictly losing the information in that case. There are still MCParticle records, the associations are not entirely ambiguous if the hits are separate and can be matched to projections. That was partially the source of my original confusion. |
Briefly, what does this PR introduce?
Currently, SimCalorimeterHitProcessor links primary generated particles with the detector hits, which takes care of early showering particles leading to calorimeter hits being associated with a large number of particles. However, this inadvertently also gets rid of e.g. single photons radiated in the detector. This PR changes the primary particle lookup so it stops when it encounters a particle where the parent is a primary and the primary only has two daughters. This should be gentle enough to not re-introduce the previous issue we dealt with when introducing the primary lookup while adding most of the crucial information related to the originator of calorimeter clusters back in.
What kind of change does this PR introduce?
Please check if this PR fulfills the following:
Does this PR introduce breaking changes? What changes might users need to make to their code?
Does this PR change default behavior?